fix: harden teardown recovery and file descriptor handling - #42
Open
ruby-dlee wants to merge 14 commits into
Open
fix: harden teardown recovery and file descriptor handling#42ruby-dlee wants to merge 14 commits into
ruby-dlee wants to merge 14 commits into
Conversation
…ned worktrees Defect 1: fm_run_bounded's command child inherits all fds from the parent process. The boundary walker in treehouse return opens one fd per worktree directory; on large trees the inherited fds consume headroom and cause EMFILE. Close every fd above stderr in the command child before exec so the bounded command starts with a clean table. Defect 2: after a manual treehouse return (workaround for defect 1), teardown refuses because the lease is cleared. Recognize an already-returned worktree as a terminal state when all other identity checks pass and work is provably landed, skipping only the treehouse return step. Refuse when work is NOT landed (fail-closed). Add regression tests for both defects plus an already-returned-but- unlanded refusal test.
The test_missing_treehouse_entry_refuses_cleared_lease case was registered in the focused block but omitted from the normal invocation list, so routine test runs skipped the fail-closed regression. Found by no-mistakes review round 4.
Extend the already-returned worktree path to cover partial returns where the directory is gone or git state is broken after a failed manual return attempt: - treehouse_lease_is_cleared: accept entries with absent leased field (partial-return state where treehouse cleared the entry incompletely) - validate_teardown_target_identity: detect directory-gone + lease-cleared and directory-exists-but-git-broken + lease-cleared, both signaling TEARDOWN_WORKTREE_ALREADY_RETURNED instead of hard-failing - cleanup_already_returned_worktree_locked: fall back to HEAD when git rev-parse fails (partial return deregistered the worktree from git) - cleanup_already_returned_worktree_no_directory: new function for the directory-gone case — revalidates lease, cleans task branch from project - Call site restructured to route directory-gone already-returned through the no-directory path instead of skipping cleanup entirely No safety check is weakened: landed-work proofs, uncommitted-change detection, and fail-closed lease verification all run unconditionally.
Remove unused `wt_path` variable from test_already_returned_directory_gone_cleans_metadata to fix SC2034 (variable assigned but never used).
ruby-dlee
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Intent
The developer wanted two fail-closed fixes to firstmate teardown: eliminate an inherited file-descriptor leak in the bounded command child so Treehouse returns work under low descriptor limits, and allow bookkeeping cleanup for already or partially returned worktrees only when the work is provably landed. They required regression coverage under a deliberately low ulimit, tests proving unlanded work is still refused, and preservation of all existing landed-work, uncommitted-change, lease, and git-identity safety checks. After reviewing the implementation, they explicitly said the technical work was correct and instructed the agent only to fix the newly introduced shell-lint failure, re-raise the branch through the no-mistakes gate, and stop. They also required terminating the agent’s running shells first and would not accept merging this teardown change while the no-mistakes gate check remained bypassed or failing.
What Changed
Risk Assessment
✅ Low: The follow-up correctly serializes the complete directory-gone recovery sequence using the project’s shared Git checkout lock and adds focused regression coverage, with no remaining material source risks identified.
Testing
Completed 1 recorded test check.
Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 1 issue found → auto-fixed ✅
bin/fm-teardown.sh:1452- The directory-gone recovery path inspects and force-deletes the task branch without holding the repository’s shared checkout lock. A concurrent Treehouse checkout can occur after the final lease check and reuse the branch/path, allowing teardown to delete newly active work. Run the complete lease-check/branch-inspection/deletion sequence under the lock keyed fromPROJ(the missingWTcannot provide lock identity).🔧 Fix: Serialize returned-worktree cleanup under checkout lock
✅ Re-checked - no issues remain.
command -v tmux >/dev/null || { echo "tmux is required for e2e tests" >&2; exit 1; }; tmux -V; rc=0; for t in tests/*.test.sh; do echo "== $t =="; bash "$t" || rc=1; done; uv run --directory tools/agent-fleet --locked pytest || rc=1; uv run --directory tools/agent-fleet --locked python -m compileall -q src || rc=1; exit "$rc"✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.